support arbitrary string parallel snapshotting#4522
Conversation
bd316b6 to
3c9cf7d
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ Test FailureAnalysis: The PR's own new feature test (Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot) fails deterministically across all three relevant suites with a value-correctness mismatch (expected 8, got 1), indicating a real bug in the arbitrary-string parallel-snapshot logic rather than a flake. |
❌ Test FailureAnalysis: A real bug: the new Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot test (the exact feature this branch implements) fails deterministically across two suite variants with an identical partition-count assertion mismatch (expected 8, got 1), indicating the arbitrary-string-key parallel snapshot isn't splitting into multiple partitions — no timeout, race, or network signature. |
3c9cf7d to
87683c6
Compare
❌ Test FailureAnalysis: The new Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot test added by this PR fails deterministically across all four CH suite variants with a consistent partition-key string value mismatch, indicating a real logic bug in the PR's arbitrary-string parallel snapshot feature rather than a flaky/timeout/race failure. |
87683c6 to
4b4de52
Compare
❌ Test FailureAnalysis: The new test Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot (matching the PR's own feature branch) fails deterministically with assertion mismatches on partition/row counts across all three suites and matrix jobs, indicating a real bug in the PR's new feature rather than flakiness. |
4b4de52 to
2817e2e
Compare
❌ Test FailureAnalysis: The PR's own new test Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot fails deterministically with partition/row-count assertion mismatches across every suite and compatibility matrix, indicating a real bug in the arbitrary-string-parallel-snapshot feature rather than flakiness. |
2817e2e to
29d182c
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
29d182c to
83aeb72
Compare
| ctx context.Context, tableName string, quotedCol string, start string, end string, | ||
| ) (uint64, error) { | ||
| query := fmt.Sprintf( | ||
| "EXPLAIN FORMAT=TRADITIONAL SELECT 1 FROM %[1]s WHERE %[2]s >= '%[3]s' AND %[2]s < '%[4]s'", |
There was a problem hiding this comment.
without FORMAT=TRADITIONAL, on newer version of mysql it will return:
mysql> explain select 1 from str_test where id > 'a' and id <= 'z';
+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| EXPLAIN |
+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| -> Filter: ((str_test.id > 'a') and (str_test.id <= 'z')) (cost=18.9 rows=93)
-> Covering index range scan on str_test using PRIMARY over ('a' < id <= 'z') (cost=18.9 rows=93)
|
+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
what we want:
mysql> explain format=traditional select 1 from str_test where id > 'a' and id <= 'z';
+----+-------------+----------+------------+-------+---------------+---------+---------+------+------+----------+--------------------------+
| id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra |
+----+-------------+----------+------------+-------+---------------+---------+---------+------+------+----------+--------------------------+
| 1 | SIMPLE | str_test | NULL | range | PRIMARY | PRIMARY | 202 | NULL | 93 | 100.00 | Using where; Using index |
+----+-------------+----------+------------+-------+---------------+---------+---------+------+------+----------+--------------------------+
❌ Test FailureAnalysis: A deterministic, pure unit test (TestBuildAdaptiveStringPartitions_MixedCase_UUID, 0.00s, no network/concurrency) newly added by this PR fails because adaptive string partitioning produced 3 partitions instead of the expected 5 for mixed-case UUIDs — a real logic bug in the PR's own feature, not flakiness. |
if fetchNextRealKey does not find a valid key; try fetchPrevRealKey before deeming the partition as unsplittable. This is useful for narrow ranges
|
The last commit 7e97b79 also fixes the flaky test scenario TestBuildAdaptiveStringPartitions_MixedCase_UUID reported above, which happens occasionally if the generated UUIDs have a midpoint that does not return a valid |
| stringPartitions, err := buildAdaptiveStringPartitions( | ||
| ctx, c, c.logger, parsedWatermarkTable, config.WatermarkColumn, start, end, numPartitions) | ||
| if err != nil { | ||
| c.logger.Warn("failed to build adaptive string partitions, falling back to full table partition", |
There was a problem hiding this comment.
We should still be deterministic in face of network errors.
Haven't we had this conversation before
#4097 (comment)
#3972 (comment)
There was a problem hiding this comment.
This masks MariaDB rejecting EXPLAIN FORMAT=TRADITIONAL
There was a problem hiding this comment.
surprised that tests that expect N partitions did not catch this, will take a look what happened here.
There was a problem hiding this comment.
MariaDB seem to support it just fine:
MariaDB [test]> explain FORMAT=TRADITIONAL SELECT 1 FROM a where id1 > 0 and id1 < 1;
+------+-------------+-------+-------+---------------+---------+---------+------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+------+-------------+-------+-------+---------------+---------+---------+------+------+-------------+
| 1 | SIMPLE | a | range | PRIMARY | PRIMARY | 4 | NULL | 1 | Using where |
+------+-------------+-------+-------+---------------+---------+---------+------+------+-------------+
There was a problem hiding this comment.
My bad on Maria, gotta keep my AIs in check
|
|
||
| partitions := make([]*protos.QRepPartition, 0, len(outputs)) | ||
| for _, p := range outputs { | ||
| partitions = append(partitions, utils.CreateStringPartition(p.start, p.end, p.end == maxVal)) |
There was a problem hiding this comment.
🟠 Major — NUL byte in a boundary key → deterministic stuck snapshot
MySQL VARCHAR permits 0x00; mysql.Escape maps it to a valid literal so probes succeed and the NUL key lands in a StringRange boundary. Postgres then rejects it: the catalog insert of partition_start/partition_end (text columns, monitoring.go:393) fails with invalid byte sequence for encoding "UTF8": 0x00 (reproduced live against the dev catalog). GetQRepPartitions fails, Temporal recomputes identical partitions, fails identically — stuck forever. Bounded by defaults: with PEERDB_OFFLOAD_PARTITION_RANGES=true (default) and InitialCopyOnly snapshots, ranges are offloaded to encrypted bytea and this insert is skipped. Trigger requires offload disabled or a continuous mirror, plus a NUL in the min/max key.
| for len(seen) < numRows { | ||
| keyBytes := make([]byte, 10) | ||
| for i := range keyBytes { | ||
| keyBytes[i] = byte('a' + rand.IntN(26)) //nolint:gosec // test data |
There was a problem hiding this comment.
I think this needs a fuzz integration test (quicker than e2e but real mysql/mariadb) and leave it overnight/weekend. LLMs can help with throwing in charsets/collations/index types (idk if relevant for PK)/padding and setting up the data generation.
| ) (string, bool, error) { | ||
| query := fmt.Sprintf( | ||
| "SELECT %[1]s FROM %[2]s WHERE %[1]s >= '%[3]s' AND %[1]s > '%[4]s' AND %[1]s < '%[5]s' ORDER BY %[1]s LIMIT 1", | ||
| quotedCol, tableName, mysql.Escape(midpoint), mysql.Escape(start), mysql.Escape(end)) |
There was a problem hiding this comment.
🔴 Critical — Watermark values interpolated with the wrong escaping for the session's sql_mode
flow/connectors/mysql/qrep_partition.go:281 (and :294, :325, plus the pull query at flow/connectors/mysql/qrep.go:379)
Every new probe query and the StringRange pull query build SQL literals with mysql.Escape(...) wrapped in single quotes. But the connector unconditionally runs SET sql_mode = 'ANSI,NO_BACKSLASH_ESCAPES' (mysql.go:237). mysql.Escape does backslash escaping (' → \'), which under NO_BACKSLASH_ESCAPES is a no-op — the backslash is a literal character. This was masked at merge-base because non-UUID strings fell back to full-table and emitted no per-value SQL; UUID boundaries are hex-only. This branch routes arbitrary row data through these literals for the first time. Three concrete failure modes, reproduced live on MySQL 8.0:
- Silent data loss (this is the critical core). A max watermark value like
Z:\tmp(Windows/UNC path as key) is stored unescaped, so partitioning succeeds — no fallback. The last partition's inclusive bound becomesBETWEEN ... AND 'Z:\\tmp', which underNO_BACKSLASH_ESCAPESis the literal 7-char stringZ:\\tmp. The realZ:\tmpsorts above it, so the max row (and neighbors) are silently never replicated. Verified:SELECT ... BETWEEN 'Mmm' AND 'Z:\\tmp'returned onlyMmm, dropping the real max. Symmetric at the min bound with\n/\t/\0. Interior boundaries stay consistent (adjacent partitions share the identical escaped key), so the leak is concentrated at the two endpoints — but it's a successful query with no error and no fallback. - SQL injection. A source row value like
x' OR '1'='1escapes to'x\' OR '1'='1'; the backslash is literal, the string closes early, and the tail executes. Watermark values are attacker-influenceable table data. Reachable but harder — quote-bearing values usually syntax-error the probe phase first (see below). - Quote-free data breaks too.
stringMidpointsynthesizes bytes in[0x20,0x7E]as base95 digits — including0x27 (')and0x5C (\). Verified:stringMidpoint("&&&", "(((")="'''", invented from quote-free input. The probe then throwsERROR 1064and the build silently degrades to a single full-table partition.
Fix: use parameterized Execute(query, args...) (already supported at mysql.go:303) or '' quote-doubling instead of mysql.Escape. This fixes all three at once.
Add parallel snapshotting support for arbitrary string using an adaptive bisection strategy.
The partitions are determined by starting with a single partition using min/max fetched values; and then continuously bisecting the largest partition into 2 smaller partitions, until we reached the target number of partitions or until they are no longer splittable.
There are known edge cases that can lead to skewed partitions (see unit tests), but never compromises on correctness. We sacrifice some balancing optimization for a more simple, deterministic partitioning algorithm.
Fixes: DBI-885